src/acquisition-sdk/__tests__/acquisition-sdk.test.ts (1)

266-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the duplicated assignment.

acquisition = acquisition = new ... assigns twice.

🧹 Proposed fix

-            acquisition = acquisition = new acquisitionSdk.AcquisitionManager(new mockApi.CustomResponseHttpRequester(invalidJsonResponse), configuration);
+            acquisition = new acquisitionSdk.AcquisitionManager(new mockApi.CustomResponseHttpRequester(invalidJsonResponse), configuration);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/acquisition-sdk/__tests__/acquisition-sdk.test.ts` at line 266, Remove
the duplicated assignment in the AcquisitionManager setup so the acquisition
variable is assigned to the new AcquisitionManager instance only once.

src/acquisition-sdk/acquisition-sdk.ts (2)

216-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace the arguments-based callback reassignment.

This line assigns false to callback when the last argument is not a function, which contradicts the declared Callback<void> type and relies on arguments inside a class method. The declared callback parameter already carries the value. Removing the line keeps behavior for all documented call shapes and improves readability.

♻️ Proposed refactor

-        callback = typeof arguments[arguments.length - 1] === "function" && arguments[arguments.length - 1];
-
         this._httpRequester.request(Http.Verb.POST, url, JSON.stringify(body), (error: Error, response: Http.Response): void => {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/acquisition-sdk/acquisition-sdk.ts` at line 216, Remove the
arguments-based reassignment of callback in the acquisition method, keeping the
declared callback parameter unchanged. Preserve the existing handling for all
documented call shapes without introducing a false value or relying on
arguments.

---

143-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Pass an explicit null error instead of the outer error variable.

The error identifier here resolves to the requester callback parameter from Line 117, which is already known to be falsy. The catch binding at Line 138 is not in scope. The current code reads as if it forwards a parse error, but it always reports "no update". Make the intent explicit.

♻️ Proposed refactor

             if (!updateInfo) {
-                callback(error, /*remotePackage=*/ null);
+                callback(/*error=*/ null, /*remotePackage=*/ null);
                 return;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/acquisition-sdk/acquisition-sdk.ts` around lines 143 - 145, Update the
!updateInfo branch in the acquisition request callback to pass an explicit null
error to callback, preserving the null remotePackage argument and early return.
Do not use the outer error parameter, since this path represents no available
update rather than a parsing failure.

src/acquisition-sdk/__tests__/acquisition-rest-mock.ts (2)

76-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Server.onAcquire is dead code.

No caller references onAcquire. It also uses field names that no longer match the SDK contract (params.deploymentKey, isAvailable), so it would mislead a future reader who wires these tests into a runner. Consider removing it during the port.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/acquisition-sdk/__tests__/acquisition-rest-mock.ts` around lines 76 - 89,
Remove the unused Server.onAcquire method from the acquisition REST mock; no
callers reference it, and its outdated deploymentKey/isAvailable contract should
not remain as misleading dead code.

---

9-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Mutable module-level exports depend on CommonJS output.

Tests reassign mockApi.serverUrl and mockApi.latestPackage through a namespace import. That works with CommonJS emit, but native ES modules make namespace properties read-only, so the assignment throws in strict mode. If the future test runner uses native ESM, replace these export var bindings with exported setter functions.

As per path instructions, tests under src/acquisition-sdk/__tests__/ are reference tests and are not wired into a runner yet, so this is a note for the runner work rather than a blocker.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/acquisition-sdk/__tests__/acquisition-rest-mock.ts` around lines 9 - 32,
Replace the mutable module-level exports serverUrl and latestPackage with
exported setter functions, and update test consumers to call those setters
instead of reassigning namespace-import properties. Preserve the existing mock
state and ensure updateMockUrl continues rebuilding URLs from the current server
URL.

Source: Path instructions

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/acquisition-sdk/acquisition-sdk.ts:242

• When API calls are disabled, reportStatusDownload invokes callback unconditionally even though it is optional, which can throw if a caller omits it.

        if (AcquisitionManager._apiCallsDisabled) {
            console.log(`[CodePush] Api calls are disabled, skipping API call`);
            callback(/*error*/ null, /*not used*/ null);
            return;
        }